Skip to content

fix(capture): honor cancellation during screen-recording permission retry#280

Closed
SebTardif wants to merge 2 commits into
openclaw:mainfrom
SebTardif:fix/screencapture-permission-honor-cancel
Closed

fix(capture): honor cancellation during screen-recording permission retry#280
SebTardif wants to merge 2 commits into
openclaw:mainfrom
SebTardif:fix/screencapture-permission-honor-cancel

Conversation

@SebTardif

@SebTardif SebTardif commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Screen-recording permission probing in ScreenRecordingPermissionChecker sleeps with try? await Task.sleep after a transient ScreenCaptureKit denial, then always runs a second SCShareableContent probe. try? swallows CancellationError, so a canceled permission check can still burn a second SCK probe instead of returning promptly.

Same cancel-swallow class as #279 / #271 / #270.

Evidence

Before

try? await Task.sleep(nanoseconds: delay)
// second currentShareableContent() always runs

After

try? await Task.sleep(nanoseconds: delay)
guard !Task.isCancelled else { return false }
// second probe only if still running

CHANGELOG.md is left to the release process (removed from this PR per review).

Live terminal transcript (after fix)

$ swiftc -parse-as-library -o /tmp/prove-sck-perm scripts/prove-screencapture-permission-cancel.swift
$ /tmp/prove-sck-perm
=== Screen-recording permission probe cancel proof (PR #280) ===
Pattern matches ScreenRecordingPermissionChecker.hasPermission

Test 1: Unfixed — cancel after 50ms into 350ms delay
  probes=2 elapsed_ms=55 granted=false
Test 2: Fixed — cancel after 50ms into 350ms delay
  probes=1 elapsed_ms=52 granted=false

PASS: unfixed ran second SCShareableContent probe after cancel (2 probes)
PASS: fixed stopped at first probe without second SCK call (1 probe, 52ms)

Production helper with injectable probe (commit on this branch):

$ swift test --package-path Core/PeekabooAutomationKit --filter ScreenRecordingPermissionCancelTests
Test Case '…testCancelDuringTransientPermissionRetrySkipsSecondProbe' passed (0.052 seconds).
Test Case '…testNonCancelledTransientPermissionRetryProbesTwice' passed (0.369 seconds).
Test Suite 'ScreenRecordingPermissionCancelTests' passed
Executed 2 tests, with 0 failures

Real behavior proof

  • Behavior or issue addressed: Canceling during the screen-recording permission transient-denial sleep must not run a second ScreenCaptureKit shareable-content probe.

  • Real environment tested: macOS 26.5.2 (Build 25F84), arm64, Peekaboo worktree on this PR branch, Swift 6.x.

  • Exact steps or command run after this patch:

    swiftc -parse-as-library -o /tmp/prove-sck-perm scripts/prove-screencapture-permission-cancel.swift
/tmp/prove-sck-perm
swift test --package-path Core/PeekabooAutomationKit --filter ScreenRecordingPermissionCancelTests

  • Evidence after fix: Terminal transcript above: unfixed path runs 2 probes after cancel; fixed path stops at 1 probe in ~52ms and returns not-granted. Production ScreenRecordingPermissionChecker with injected probe matches the same counts.

  • Observed result after fix: Canceled permission checks return false after one SCK probe. Non-cancelled transient denials still probe twice.

  • What was not tested: Live TCC denial on a signed Peekaboo binary with real ScreenCaptureKit (requires denying Screen Recording in System Settings during a live capture). Delay length matches ScreenCaptureKitTransientError (350ms).

Summary

  • Post-sleep Task.isCancelled guard on the permission retry path.
  • Injectable preflight/probe seams for deterministic regression coverage.
  • Standalone prove script for terminal red/green comparison.
  • No CHANGELOG.md edit (release-owned).

Related: #279, #271, #270

@SebTardif
fix(capture): honor cancellation during screen-recording permission r…
a1d740f 
…etry

The permission probe retry slept with try?, so a canceled caller could
still run a second SCShareableContent probe. Recheck cancellation after
sleep and return false without another probe.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jul 16, 2026
@clawsweeper

clawsweeper Bot commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 7:17 AM ET / 11:17 UTC.

Summary
Adds a post-sleep cancellation guard to the screen-recording permission retry path and adds a changelog entry.

Reproducibility: no. there is not yet a high-confidence real-environment reproduction. Source inspection clearly exposes the swallowed-cancellation path, but the PR does not exercise a live or focused production-helper cancellation during the ScreenCaptureKit retry.

Review metrics: 2 noteworthy metrics.

  • Diff Surface: 2 files, 5 additions, 0 deletions. The behavioral change is tightly scoped, while one of the two files is release-owned metadata rather than implementation.
  • Regression Coverage: 0 test files changed. The cancellation behavior is subtle and the existing safe suite does not directly demonstrate the affected retry path.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Provide redacted terminal output or runtime logs demonstrating that cancellation during retry backoff prevents a second ScreenCaptureKit probe.
  • Remove the CHANGELOG.md edit.
  • [P1] Add focused regression coverage for cancellation during the retry sleep if the production helper can be exercised deterministically.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR provides green lint, tests, and source reasoning but no after-fix live or focused production-helper evidence for a canceled ScreenCaptureKit permission retry; add redacted terminal output or runtime logs, update the PR body to trigger re-review, or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] Without a focused live or production-helper demonstration, reviewers cannot confirm that cancellation during the transient TCC backoff prevents the second real ScreenCaptureKit probe while preserving the expected non-cancelled retry behavior.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the post-sleep guard, add focused cancellation regression coverage, provide redacted terminal or runtime evidence showing that cancellation suppresses the second probe, and leave CHANGELOG.md to the release process.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The remaining merge gate is contributor-supplied real behavior proof from a macOS ScreenCaptureKit setup, plus removal of the release-owned changelog entry; automation cannot manufacture the required environment evidence.

Security
Cleared: The patch adds only local cancellation control flow and release text, with no dependency, credential, permission, download, or supply-chain changes.

Review findings

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:9
Review details

Best possible solution:

Keep the post-sleep guard, add focused cancellation regression coverage, provide redacted terminal or runtime evidence showing that cancellation suppresses the second probe, and leave CHANGELOG.md to the release process.

Do we have a high-confidence way to reproduce the issue?

No, there is not yet a high-confidence real-environment reproduction. Source inspection clearly exposes the swallowed-cancellation path, but the PR does not exercise a live or focused production-helper cancellation during the ScreenCaptureKit retry.

Is this the best way to solve the issue?

Yes, the immediate post-sleep cancellation check matches established repository patterns and is the narrowest correction. Merge readiness still requires direct behavior proof and removal of the release-owned changelog edit.

Full review comments:

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:9
    Normal pull requests should leave CHANGELOG.md to the release process and keep release-note context in the PR body or commit message. This line was already present at the previously reviewed head, so this is a late policy finding; remove it before merge.
    Confidence: 0.99
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is correct
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against e2a46347138d.

Label changes

Label justifications:

  • P2: This is a narrow cancellation correctness fix with limited blast radius and no evidence of data loss, security bypass, or core-runtime unavailability.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR provides green lint, tests, and source reasoning but no after-fix live or focused production-helper evidence for a canceled ScreenCaptureKit permission retry; add redacted terminal output or runtime logs, update the PR body to trigger re-review, or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

What I checked:

Likely related people:

  • SebTardif: Previously authored several merged cancellation fixes in adjacent polling and window-control paths, making them closely familiar with the established implementation pattern beyond this proposal. (role: recent cancellation-pattern contributor; confidence: high; commits: b6a089828101, ee03f0df87b5, ed1a72186cd3; files: Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/Capture/ScreenCapturePermissionGate.swift)
  • romanr: Prior repository history associates this contributor with the screen-recording permission reliability work that introduced or shaped this capture-permission surface. (role: screen-recording permission feature contributor; confidence: medium; files: Core/PeekabooAutomationKit/Sources/PeekabooAutomationKit/Services/Capture/ScreenCapturePermissionGate.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-16T13:09:26.973Z sha a1d740f :: needs real behavior proof before merge. :: none

@SebTardif

SebTardif commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Removed the release-owned CHANGELOG entry. Added injectable probe seams, focused cancel/non-cancel regression tests (1 vs 2 probes), and a red/green terminal prove script showing unfixed burns a second SCShareableContent probe after cancel while fixed stops at one. PR body updated with live transcript.

@clawsweeper

clawsweeper Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@steipete steipete mentioned this pull request Jul 21, 2026
Closed
@steipete

steipete commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Triage Wave 5 closeout: closing without merge, not requesting changes.

This is the same swallowed-cancellation class as #279, at the adjacent ScreenRecordingPermissionChecker owner boundary. A correct canonical change should cover the two fallback-runner sleeps from #279 and this permission-probe sleep together, with shared proof and one changelog entry, rather than land two overlapping PRs.

The maintainer reconstruction confirmed the bug on current main: cancellation caused 2 permission probes before the fix and 1 after it, while the non-canceled transient-denial control still retried exactly once. The combined local patch passed focused tests and canonical AutoReview with no actionable findings. Contributor head ece4a67843d9e4be5ee7ae7bd9be54eff3b0d8d5 also has hosted macOS CI run 29779902730 green across all 5 jobs.

It cannot be merged under the required live-proof bar. On the available macOS host, the signed production GUI/on-demand paths timed out despite mandatory permissions reporting granted, and a current-Developer-ID-signed XCTest host could not get a real SCShareableContent denial call to return. Restarting the host and replayd did not recover the boundary. The standalone proof script and injected probe tests therefore remain deterministic regression evidence, not live ScreenCaptureKit/TCC evidence.

No commit was pushed and there is no merge SHA. Closing as overlap with #279 plus the same mandatory live-boundary blocker. Thanks @SebTardif for identifying the permission-probe sibling and adding targeted coverage.

@steipete steipete closed this Jul 21, 2026
This was referenced Jul 21, 2026
Closed
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SebTardif @steipete